Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

work-in-progress: redis integration #1

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

stock90975
Copy link

Lots of updates:

README.md

  • started on docs (wip)

example_redis.py

  • I wasn't sure where to put this
  • just for testing & playing around with setting/getting values
  • shows "easy" get/set
  • shows direct access to redis connection to execute actual Redis commands

lona_redis/middlewares.py

  • a first pass of how it might / would work
  • I know you said that you wanted smaller multiple settings keys, but Redis has so many kwargs for setting up the connection (https://redis.readthedocs.io/en/latest/connections.html). I'm not sure how to reconcile - open to suggestions / direction

tests

  • I know I should but I don't know how to do them in this case, with the browser interactivity :-(

Everything is open for comment / changes

Thanks!

@stock90975
Copy link
Author

On reflection, I think the .get & .set parameters should be more consistent with each other: ie:

request.user.session.set("foo",999)
foo = request.user.session.get("foo")

@fscherf
Copy link
Member

fscherf commented Apr 21, 2023

@stock90975: Great job! This will need some more thought and work, but I think we are heading in the right direction here :) I already have some questions / feedback:

  1. Settings keys: A I see! I agree, in this case, it would create to much overhead and maintenance work, splitting all settings in separate keys. settings.REDIS_SETTINGS will be fine in this case.
  2. RedisUser: You added FIXME this isn't used - can be removed? to the code. We will need some kind of user object that can be compared to others, so Lona can determine if a user may see a view or not, when a view is a daemon-view (https://github.com/lona-web-org/lona/blob/master/lona/view_runtime_controller.py#L42)
  3. Why is pickle used?

@stock90975
Copy link
Author

@stock90975: Great job! This will need some more thought and work, but I think we are heading in the right direction here :)

@fscherf Thanks!!!

  1. Settings keys: A I see! I agree, in this case, it would create to much overhead and maintenance work, splitting all settings in separate keys. settings.REDIS_SETTINGS will be fine in this case.

Glad you agree, I should have elaborated. If we pick some Redis settings, then

  • who decides which ones? The "important" ones? What's "important" to who?
  • what if Redis adds more options? We will be chasing our tail ...
    However ... the user could put in stupid values and crash, but we can't stop that anyway right?
  1. RedisUser: You added FIXME this isn't used - can be removed? to the code. We will need some kind of user object that can be compared to others, so Lona can determine if a user may see a view or not, when a view is a daemon-view (https://github.com/lona-web-org/lona/blob/master/lona/view_runtime_controller.py#L42)

Hmm ... I need to study this to understand how lona works 😀

  1. Why is pickle used?

Redis (and redis-py) is "limited" in what datatypes can be stored, eg:

# start first Redis with:
# docker run -p 6379:6379 -it redis:latest

import redis
r = redis.Redis(host='localhost', port=6379, db=0)
r.set('foo', True)
# redis.exceptions.DataError: Invalid input of type: 'bool'. Convert to a bytes, string, int or float first.

🙄
BTW by default Redis returns values as byte strings, so the user has to further massage the return value.

Even most basic Python datatypes are not accepted "as-is", let alone dicts or list or other wierd combos. My examples demonstrate that lona-redis could cope with these easily. Anything pickle-able will work. Hopefully users will only want to store pickle-able values!

Using pickle IS a sledgehammer approach, but I coudn't think of a better way. I'm definitely open to suggestions, as this is a possible speed penalty, that might detract from using Redis in the first place.

Redis has some super-sophisticated powers! (https://redis.readthedocs.io/en/latest/commands.html#) For this reason also, I think lona-redis should expose the r. object to the user so they can easily get to these commands if they really want to.

Hence, the dual approach: use lona-redis the "easy" way or the hardcore way...

@stock90975
Copy link
Author

RedisUser: You added FIXME this isn't used - can be removed? to the code. We will need some kind of user object that can be compared to others, so Lona can determine if a user may see a view or not, when a view is a daemon-view (https://github.com/lona-web-org/lona/blob/master/lona/view_runtime_controller.py#L42)

I my mymiddlewares.py:

    def handle_request(self, data):
        view = data.view
        # view.is_daemon is always False

even though in the view

    def handle_request(self, request):
        self.is_daemon = True

How do I check if a view is a daemon-view from middlewares?

@fscherf
Copy link
Member

fscherf commented Apr 24, 2023

@stock90975

Glad you agree, I should have elaborated. If we pick some Redis settings, then

  • who decides which ones? The "important" ones? What's "important" to who?
  • what if Redis adds more options? We will be chasing our tail ...
    However ... the user could put in stupid values and crash, but we can't stop that anyway right?

I think we then should go with your first approach and just have settings.REDIS_SETTINGS as a dict, that gets passed to redis.Redis as is. redis.Redis then has to check if the arguments are valid or not, and we don't have to worry about
newly added settings.

On the pickle question: Ok, makes sense to me for now. To be honest, I never used redis-py before. We will see if this creates performance problems down the line, but I think its ok for now.

On the middleware/daemon question: Middleware.handle_request() runs before, View.handle_request(), and View.handle_request() is the method where you would run self.is_daemon = True. So the middleware will always see view.is_daemon == False.
Why do you want to check this in the middleware?

@stock90975
Copy link
Author

I've used a Redis in Flask - when trying to use it directly, it got quite ugly. I believe this implementation will give the user better access to the full functionality, but without sacrificing ease of use.

Why do you want to check this in the middleware?

Just trying to understand how views work to address #2:

RedisUser: You added FIXME this isn't used - can be removed? to the code. We will need some kind of user object that can be compared to others, so Lona can determine if a user may see a view or not, when a view is a daemon-view

TBH I'm a bit lost as to how to do this because I don't get how views would affect the use of lona-redis, so I'm stuck as to how to proceed here.

I will clean up my code for the Redis part of the lona-redis middleware and write up usage docs soon.

@fscherf
Copy link
Member

fscherf commented Apr 24, 2023

TBH I'm a bit lost as to how to do this because I don't get how views would affect the use of lona-redis, so I'm stuck as to how to proceed here.

Could it be that we mean different things when we say redis session? I thought of it like in authentication. lona-django uses Django's authentication system by creating a custom user class that can be set to request.user. Lona then can do a == comparison to check two requests came from the same, high-level, Django-user. You then can get and set session data, using Django's session system.

@stock90975
Copy link
Author

Hmm, I was thinking more as cookies, using Redis as the key-value store. Anyway, please have a look at the latest code & docs and let me know what you think.

I was hoping for just a basic cookie set() & get() that lona could do out of the box. (Maybe it does but I just don't know how...)

@fscherf
Copy link
Member

fscherf commented Apr 30, 2023

@stock90975: But every user should have its own key-value store right? Lona identifies each user by a session cookie, which gets randomly generated on the first connect (no username-password pair). Should we use this here too, or should we implement some password infrastructure like in lona-django?

The code looks ok, but the commits are pretty messy

@stock90975
Copy link
Author

stock90975 commented May 2, 2023

Sorry about the commits - I'm learning git. Should I be commiting each file individually or multiple files as a commit?

But every user should have its own key-value store right?

Yes I'm using request.user.session_key
(https://github.com/lona-web-org/lona-redis/pull/1/files#diff-82df38ea83e69db20c582f12ce69fe73ccecaad93941697e68b32c1c13d28f74R172)

and redis_key()
https://github.com/lona-web-org/lona-redis/pull/1/files#diff-82df38ea83e69db20c582f12ce69fe73ccecaad93941697e68b32c1c13d28f74R31
is pre-pending the session_key to the user's key. eg:
user uses "foo" as the key, but actually "abcd1234:foo" is the actual key used to store the value

Since request.user.session_key is unique, this should distinguish one user's key from another user's key right? (I think...)

@fscherf
Copy link
Member

fscherf commented May 4, 2023

Sorry about the commits - I'm learning git. Should I be commiting each file individually or multiple files as a commit?

No problem! I am sorry, If I was a bit harsh. Every commit should do only one thing, like fix one bug, or add one feature, regardless of how many files are changed. It's often hard to tell what "one thing" is. For example, I am currently working on channels for Lona. That changes many files at once, but these changes only work in conjunction, so it makes no sense to split them into multiple commits.

In this case I am completely fine with one big commit with a commit message like "initial code commit", because every "change" is an "add" currently.

Yes I'm using request.user.session_key (https://github.com/lona-web-org/lona-redis/pull/1/files#diff-82df38ea83e69db20c582f12ce69fe73ccecaad93941697e68b32c1c13d28f74R172)

and redis_key() https://github.com/lona-web-org/lona-redis/pull/1/files#diff-82df38ea83e69db20c582f12ce69fe73ccecaad93941697e68b32c1c13d28f74R31 is pre-pending the session_key to the user's key. eg: user uses "foo" as the key, but actually "abcd1234:foo" is the actual key used to store the value

Since request.user.session_key is unique, this should distinguish one user's key from another user's key right? (I think...)

Ok great! So Lona can tell apart two different user then. To do so we have provide Lona a custom User model which implements __eq__ (custom == handling).

Basically Lona does this:

def new_connection_comes_in(connection, view):
    if connection.user == view.user:
        view.add_connection(connection)
    else:
        raise ForbiddenError

@stock90975
Copy link
Author

Ok thanks for understanding! I really am a noob ...

Ok, so what is the next step?

BTW Further to using Redis as the backend, do you think it would it be useful / good / attractive for those new to lona to see that it can deal with a session cookies out-of-the-box? Specifically I'm thinking of having a version that does not need Redis, for really simple or demo purposes. eg. the backend for the key-value pairs is just a Python dict. eg. this case would be triggered if settings.REDIS_SETTINGS was missing.

@fscherf
Copy link
Member

fscherf commented May 16, 2023

BTW Further to using Redis as the backend, do you think it would it be useful / good / attractive for those new to lona to see that it can deal with a session cookies out-of-the-box? Specifically I'm thinking of having a version that does not need Redis, for really simple or demo purposes. eg. the backend for the key-value pairs is just a Python dict. eg. this case would be triggered if settings.REDIS_SETTINGS was missing.

Lona is all about being as batteries-included with useful defaults as possible so I would want lona-redis to be able to handle sessions, but it should be opt-in, so you will have to switch it on.

What do you mean by "a version that does not need redis"? Isn't this library all about integrating redis? If you want a simple key-value store, you already can use server.state (https://lona-web.org/1.x/cookbook/using-server-state.html)

@stock90975
Copy link
Author

What do you mean by "a version that does not need redis"? Isn't this library all about integrating redis? If you want a simple key-value store, you already can use server.state (https://lona-web.org/1.x/cookbook/using-server-state.html)

Oh I see, I didn't think about using server.state!
... but server.state isn't unique to the session, right? In order to make it unique to the session, wouldn't it have to be somehow incorporated with request.user.session_key?

@fscherf
Copy link
Member

fscherf commented May 26, 2023

Oh I see, I didn't think about using server.state! ... but server.state isn't unique to the session, right? In order to make it unique to the session, wouldn't it have to be somehow incorporated with request.user.session_key?

Yes it is global, but you can do something like this:

per_user_data = server.state.get(request.session.session_key, {})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants